[2.2] Support for receiving commands to manipulate Rollouts #4040

douglascamata · 2022-01-24T14:39:53Z

Description

This pull requests add support Rollouts manipulation commands sent by Ambassador's backend through Agent directives and reports back with results of their execution.

Currently 3 commands are supported:

Pause a rollout.
Resume rollout.
Abort rollout.

The report includes:

The command ID
Whether it executed successfully or not
A message (error message in case it didn't execute successfully)

Related Issues

List related issues.

Testing

Automated tests were added to the rollout command execution logic
Manual integration tests were done using a stub of Ambassador's cloud to ensure the commands will be executed as expected and their results correctly reported.

Checklist

I made sure to update CHANGELOG.md.

Remember, the CHANGELOG needs to mention:
- Any new features
- Any changes to our included version of Envoy
- Any non-backward-compatible changes
- Any deprecations
This is unlikely to impact how Ambassador performs at scale.

Remember, things that might have an impact at scale include:
- Any significant changes in memory use that might require adjusting the memory limits
- Any significant changes in CPU use that might require adjusting the CPU limits
- Anything that might change how many replicas users should use
- Changes that impact data-plane latency/scalability
My change is adequately tested.

Remember when considering testing:
- Your change needs to be specifically covered by tests.
  - Tests need to cover all the states where your change is relevant: for example, if you add a behavior that can be enabled or disabled, you'll need tests that cover the enabled case and tests that cover the disabled case. It's not sufficient just to test with the behavior enabled.
- You also need to make sure that the entire area being changed has adequate test coverage.
  - If existing tests don't actually cover the entire area being changed, add tests.
  - This applies even for aspects of the area that you're not changing – check the test coverage, and improve it if needed!
- We should lean on the bulk of code being covered by unit tests, but...
- ... an end-to-end test should cover the integration points
I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

LukeShu · 2022-01-31T20:24:10Z

I've rebased this on to lukeshu/upgrade-client-go and squashed a few commits that should have been squashed. The diff from a naive git merge origin/lukeshu/upgrade-client-go (which I pushed as lukeshu/merge/dcamata/remote-control-rollouts, if you want to see it) is as follows:

diff --git a/builder/Dockerfile b/builder/Dockerfile
index da85aba1a..279e6fc94 100644
--- a/builder/Dockerfile
+++ b/builder/Dockerfile
@@ -79,6 +79,7 @@ ADD pkg pkg
 ADD vendor vendor
 ADD go.mod go.mod
 ADD go.sum go.sum
+
 RUN --mount=type=cache,target=/root/.cache/go-build \
     mkdir -p /go/bin && \
 	time go build -mod=vendor -o /go/bin/ ./cmd/...
diff --git a/go.mod b/go.mod
index cb8d5f737..03fb51bcc 100644
--- a/go.mod
+++ b/go.mod
@@ -48,6 +48,7 @@ go 1.17
 require (
 	github.com/Masterminds/semver v1.5.0 // indirect
 	github.com/Masterminds/sprig v2.22.0+incompatible
+	github.com/argoproj/argo-rollouts v1.0.7
 	github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 // indirect
 	github.com/census-instrumentation/opencensus-proto v0.2.1
 	github.com/cncf/udpa/go v0.0.0-20210322005330-6414d713912e
@@ -98,8 +99,6 @@ require (
 	sigs.k8s.io/yaml v1.2.0
 )
 
-require github.com/argoproj/argo-rollouts v1.0.7
-
 require (
 	cloud.google.com/go v0.58.0 // indirect
 	github.com/Azure/go-ansiterm v0.0.0-20170929234023-d6e3b3328b78 // indirect

Signed-off-by: Douglas Camata <[email protected]>

rp4rk

LGTM

…amata/remote-control-rollouts

Signed-off-by: Douglas Camata <[email protected]>

kflynn

Need to handle merge conflicts and tests, but this looks good! 🙂 My comments below are things that don't need to block landing this.

kflynn · 2022-02-03T15:40:52Z

pkg/agent/rollouts.go

 )

+// rolloutsGetterFactory is a factory for creating RolloutsGetter.


So what's the purpose of a RolloutsGetter?

The purpose of RolloutsGetter is to fetch Rollouts in a namespace inside the cluster.

kflynn · 2022-02-03T15:41:22Z

docs/releaseNotes.yml

+      - title: Support received commands to pause, continue and abort a Rollout via Agent directives
+        type: feature
+        body: >-
+          The Emissary agent now receive commands to manipulate Rollouts (pause, continue, and abort are


This should really be "receives". 🙂

kflynn · 2022-02-03T15:44:27Z

pkg/agent/directive_handler.go

+	a.ambassadorAPIKeyMutex.Lock()
+	apiKey := a.ambassadorAPIKey
+	a.ambassadorAPIKeyMutex.Unlock()


Might be worth a comment that you really don't want to hold this lock during the report-command-result call.

@kflynn Why? Where would you recommend to use the mutex to access the api key from the agent and send it down to this point?

LukeShu

Resolve the conflicts, and it looks fine to me.

Signed-off-by: Douglas Camata <[email protected]>

…amata/remote-control-rollouts

Signed-off-by: Douglas Camata <[email protected]>

douglascamata mentioned this pull request Jan 24, 2022

Support for receiving commands to manipulate Rollouts #4039

Closed

4 tasks

douglascamata self-assigned this Jan 25, 2022

douglascamata added the t:feature New feature or enhancement request label Jan 25, 2022

douglascamata marked this pull request as ready for review January 25, 2022 10:50

douglascamata requested a review from kflynn January 25, 2022 10:50

LukeShu mentioned this pull request Jan 31, 2022

Upgrade Kube client libraries 1.20.2→1.20.4 #4066

Merged

4 tasks

douglascamata force-pushed the dcamata/remote-control-rollouts branch from cfae4fc to f7911b9 Compare January 31, 2022 18:30

douglascamata and others added 14 commits January 31, 2022 12:50

Add argo rollouts server dependency [ci-skip]

3c19e3b

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

go mod tidy && go mod vendor && make generate

6f5c7c8

Signed-off-by: Luke Shumaker <[email protected]>

Sketch a basic implementation for updating in-cluster rollouts

4e53409

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Add missing actions to RolloutCommand pb

a31d36a

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Handle rollout commands coming over grpc

94ad4e9

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Improve error handling on rollout commands

8b598c2

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Add table tests for RolloutCommand

969c75d

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Inject the Rollouts Getter creation through the Agent

b1c4d8b

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Add some comments types in rollouts.go

e2fec9a

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Add changelog entry about rollout commands

bf3854b

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Properly pass the context to run rollout commands

eb1af93

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Fix rollout action parsing

dea4c48

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Add permission for the agent to patch rollouts

1019b2d

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Report results of running rollout commands to the cloud

a64e90d

Signed-off-by: Douglas Camata <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

LukeShu force-pushed the dcamata/remote-control-rollouts branch from f7911b9 to a64e90d Compare January 31, 2022 20:22

LukeShu changed the base branch from master to lukeshu/upgrade-client-go January 31, 2022 20:22

Base automatically changed from lukeshu/upgrade-client-go to master February 1, 2022 04:34

Douglas Camata added 3 commits February 1, 2022 12:42

Authenticate command result report call

3ddb8e4

Signed-off-by: Douglas Camata <[email protected]>

Fix error log when cannot report command result

03b43da

Signed-off-by: Douglas Camata <[email protected]>

Update release notes with info about the command reports

1602547

Signed-off-by: Douglas Camata <[email protected]>

khussey changed the title ~~Support for receiving commands to manipulate Rollouts~~ [2.2] Support for receiving commands to manipulate Rollouts Feb 2, 2022

rp4rk previously approved these changes Feb 2, 2022

View reviewed changes

Douglas Camata added 2 commits February 2, 2022 16:44

Merge branch 'master' of github.com:emissary-ingress/emissary into dc…

0e97d5e

…amata/remote-control-rollouts

Update generated files

fa28a62

Signed-off-by: Douglas Camata <[email protected]>

douglascamata dismissed rp4rk’s stale review via fa28a62 February 2, 2022 16:44

kflynn previously approved these changes Feb 3, 2022

View reviewed changes

LukeShu self-requested a review February 3, 2022 18:25

LukeShu previously approved these changes Feb 3, 2022

View reviewed changes

Douglas Camata added 2 commits February 3, 2022 20:11

Fix typo in the changelog

a5a26f0

Signed-off-by: Douglas Camata <[email protected]>

Merge branch 'master' of github.com:emissary-ingress/emissary into dc…

9c07c67

…amata/remote-control-rollouts

douglascamata dismissed stale reviews from LukeShu and kflynn via 9c07c67 February 3, 2022 19:17

Update generated manifests

c71a957

Signed-off-by: Douglas Camata <[email protected]>

LukeShu self-requested a review February 3, 2022 21:15

LukeShu approved these changes Feb 3, 2022

View reviewed changes

LukeShu merged commit 7023e92 into master Feb 3, 2022

LukeShu deleted the dcamata/remote-control-rollouts branch February 3, 2022 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.2] Support for receiving commands to manipulate Rollouts #4040

[2.2] Support for receiving commands to manipulate Rollouts #4040

douglascamata commented Jan 24, 2022 •

edited

Loading

LukeShu commented Jan 31, 2022

rp4rk left a comment

kflynn left a comment •

edited

Loading

kflynn Feb 3, 2022

douglascamata Feb 3, 2022

kflynn Feb 3, 2022

kflynn Feb 3, 2022

douglascamata Feb 3, 2022

LukeShu left a comment

		)

		// rolloutsGetterFactory is a factory for creating RolloutsGetter.

[2.2] Support for receiving commands to manipulate Rollouts #4040

[2.2] Support for receiving commands to manipulate Rollouts #4040

Conversation

douglascamata commented Jan 24, 2022 • edited Loading

Description

Related Issues

Testing

Checklist

LukeShu commented Jan 31, 2022

rp4rk left a comment

Choose a reason for hiding this comment

kflynn left a comment • edited Loading

Choose a reason for hiding this comment

kflynn Feb 3, 2022

Choose a reason for hiding this comment

douglascamata Feb 3, 2022

Choose a reason for hiding this comment

kflynn Feb 3, 2022

Choose a reason for hiding this comment

kflynn Feb 3, 2022

Choose a reason for hiding this comment

douglascamata Feb 3, 2022

Choose a reason for hiding this comment

LukeShu left a comment

Choose a reason for hiding this comment

douglascamata commented Jan 24, 2022 •

edited

Loading

kflynn left a comment •

edited

Loading